-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(a11y): adjust the name and role of the multi-select #6499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Android Build Available Rocket.Chat Experimental 4.63.0.87803 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNRmNP2n5D8tH5dsgkCR4MeyJoGDkRo1m2ELCzPvQv_2bHRGcoC7am1O7z0uztkuU4Oddavp5CSM6IpgT4Km |
|
iOS Build Available Rocket.Chat Experimental 4.63.0.87803 |
|
Android Build Available Rocket.Chat Experimental 4.64.0.93303 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQrJFoEa-W0ny1Rv6Sxz60pTzlLW6px_P593THuf7cfDZo-G2QA8ZddGhq2_S6ir0B6DsTtkRM2zGaHGUjT |
|
iOS Build Available Rocket.Chat Experimental 4.64.0.100081 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.maestro/tests/assorted/join-from-directory.yaml (3)
29-30: Consider reducing the animation timeout.A 10-second timeout for
waitForAnimationToEndis quite long. If this was added to address test flakiness, consider whether a shorter duration (e.g., 2-3 seconds) would suffice, which would improve test execution time.
100-103: Consider adding state-change verification and initial state setup.The test now relies on the accessibility label format 'Users unselected', which makes it more brittle. Additionally:
- The test assumes 'Users' starts unselected but doesn't explicitly verify or set the initial filter state.
- After tapping 'Users unselected' (line 103), there's no verification that the filter actually changed state (e.g., checking for 'Users selected' or verifying the directory content changed).
This could lead to flakiness if:
- The label format changes (e.g., to '☐ Users' or 'Users (unselected)')
- Previous tests or saved state leave 'Users' already selected
- The tap doesn't successfully change the filter
Consider adding verification steps
After line 103, add a step to verify the filter changed:
- extendedWaitUntil: visible: text: 'Users selected' timeout: 5000Or verify the filter menu closed and directory content reflects the Users filter.
148-151: Same concerns as the 'Users unselected' change above.This has the same brittleness and missing verification issues as the 'Users unselected' change in lines 100-103. After tapping 'Teams unselected' (line 151), consider adding verification that the filter state changed to 'Teams selected' or that the directory content reflects the Teams filter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.maestro/tests/assorted/join-from-directory.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/views/CreateDiscussionView/index.tsx (1)
49-51: Stale state issue remains unresolved.The previous review comment about state synchronization is still applicable. The local
usersstate (line 69) is initialized from ReduxselectedUsersonce at mount but never synchronizes with subsequent Redux updates. If a user navigates away to add more users and returns, the UI will show stale data.Please refer to the previous review comment for recommended solutions (use Redux state directly or add a
useEffectto sync).Also applies to: 98-100, 116, 181
Also applies to: 53-53, 63-64, 69-69
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/containers/List/__snapshots__/List.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
app/containers/List/List.stories.tsxapp/containers/SelectedUsers/index.tsxapp/containers/ServerItem/index.tsxapp/views/CreateChannelView/index.tsxapp/views/CreateDiscussionView/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/CreateChannelView/index.tsx
- app/containers/List/List.stories.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
app/containers/ServerItem/index.tsxapp/views/CreateDiscussionView/index.tsx
🧬 Code graph analysis (1)
app/views/CreateDiscussionView/index.tsx (2)
app/reducers/selectedUsers.ts (1)
ISelectedUser(4-11)app/actions/selectedUsers.ts (1)
removeUser(25-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (8)
app/containers/ServerItem/index.tsx (4)
5-5: LGTM! Import style matches maintainer preference.The namespace import
import * as Listis the preferred style as suggested in previous reviews, and it supports the refactored icon usage.
34-36: Excellent! Correctly implements iOS VoiceOver workaround.The implementation correctly includes the selection state ("Selected"/"Unselected") directly in the
accessibilityLabelinstead of usingaccessibilityState={{ checked: hasCheck }}. This approach works around the known iOS VoiceOver issues withaccessibilityRole="radio"+accessibilityState, ensuring proper state announcement for screen reader users on iOS.The use of computed constants (
iconName,iconColor) also improves code readability.Based on learnings, this is the correct approach for iOS compatibility.
44-46: LGTM! Accessibility props properly configured.The accessibility implementation is correct:
accessibilityRole='radio'provides proper semantic identificationaccessibilityLabelincludes both identifying information and stateaccessibilityHintguides user interaction- Correctly avoids
accessibilityStateper iOS VoiceOver compatibility requirementsThis addresses the PR objectives (MA-120: correct radio button role in menus).
73-74: LGTM! Clean refactoring to List.Icon.The refactoring from a dedicated Radio component to
List.Iconimproves consistency across the codebase. The icon appearance (radio-checked/unchecked with appropriate colors) is maintained through the computed constants, and this aligns with the PR objective to consolidate components and reduce code duplication.app/views/CreateDiscussionView/index.tsx (4)
30-31: LGTM!The new imports correctly support the integration of the SelectedUsers component and proper typing for user data.
145-146: LGTM!The background color update to
surfaceTintprovides consistent theming across the view.
176-176: Good catch on the accessibility typo!Correcting
additionalAcessibilityLabeltoadditionalAccessibilityLabelimproves code correctness and aligns with proper accessibility implementation.
181-181: SelectedUsers integration looks correct.The component is properly integrated with appropriate props. However, note that the
removeUsercallback signature matches the interface issue identified inapp/containers/SelectedUsers/index.tsx(where the interface should be updated to reflect thatonPressreceives an ID string, not the full item).Once the interface fix in SelectedUsers is applied, verify this usage still compiles correctly.
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/MA-181
https://rocketchat.atlassian.net/browse/MA-120
https://rocketchat.atlassian.net/browse/MA-179
https://rocketchat.atlassian.net/browse/MA-178
https://rocketchat.atlassian.net/browse/MA-118
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.